-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cluster-ui: show database info and ability to choose statement columns #294
Conversation
838b835
to
4c26b34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 21 of 22 files at r1.
Reviewable status: 21 of 22 files reviewed, 9 unresolved discussions (waiting on @j-low, @laurenbarker, @maryliag, and @nathanstilwell)
a discussion (no related file):
@maryliag ,tried to unselect "All" checkbox and it didn't work. I assume it should unselect all options and allow users to select particular columns.
Also, when I try manually unselect all options, clicking on the last option - toggles all options back to selected state.
packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 42 at r1 (raw file):
return ( <components.Option {...props}> <input
Is it possible to reuse CheckboxInput
component from ui-components
package (packages/ui-components/src/Input/CheckboxInput.tsx
)?
packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 133 at r1 (raw file):
allOptions: OptionsType<SelectOption>, ) => { let selected = selectedOptions
@maryliag , this logic with strings manipulations looks quite complex. Could you explain the main idea behind this?
packages/cluster-ui/src/columnsSelector/columnsSelector.module.scss, line 11 at r1 (raw file):
height: 32px; width: 67px; font-size: 12px;
Default font-sizes and spacing can be reused from packages/cluster-ui/src/core/index.module.scss
(.lavel
class uses it already)
packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 76 at r1 (raw file):
totalFingerprints: number; lastReset: string; columns: string;
@maryliag , what is the main intend to store columns
as a string? Would it be better to keep it in array of strings?
packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 327 at r1 (raw file):
this.state.tableColumnsSelected == undefined || this.state.tableColumnsSelected.length == 0 || (this.state.tableColumnsSelected == "default" &&
What "default" value means?
packages/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 97 at r1 (raw file):
}, ); return Object.keys(databases);
nit. It might be more clear to rewrite it in more declarative way with filter + map functions and then get rid of duplicate values with Set.
return [
...new Set(statementsState.data.statements
.filter(s => !!s.key.key_data.database)
.map(s => s.key.key_data.database)
).values()
]
packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 155 at r1 (raw file):
displayColumns == "" || displayColumns == "default" )
nit. wrap if
statement in { ... }
to comply with linting rules
packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 159 at r1 (raw file):
return columns.filter(option => { return displayColumns.split(",").includes(option.name);
it might be refactored out from filter
predicate:
const displayCol = displayColumns.split(",");
return columns.filter(option => {
return displayCol.includes(option.name);
}
4c26b34
to
5703945
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 23 files reviewed, 9 unresolved discussions (waiting on @j-low, @koorosh, @laurenbarker, and @nathanstilwell)
a discussion (no related file):
Previously, koorosh (Andrii Vorobiov) wrote…
@maryliag ,tried to unselect "All" checkbox and it didn't work. I assume it should unselect all options and allow users to select particular columns.
Also, when I try manually unselect all options, clicking on the last option - toggles all options back to selected state.
Good point, I forgot that case! It's fixed now!
packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 42 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
Is it possible to reuse
CheckboxInput
component fromui-components
package (packages/ui-components/src/Input/CheckboxInput.tsx
)?
Weird as it sounds the CheckboxInput
is not really a checkbox, is just a regular input that I can then force to be checkbox. The problem is that it includes a wrapper div that I can't change the class and is messing up all alignments of the option, so I opted for a simple input instead.
packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 133 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
@maryliag , this logic with strings manipulations looks quite complex. Could you explain the main idea behind this?
I realized from your comment that was indeed confusing, so I restructured a little and used better names to help this issue. I also change to use array instead of string, so there is less manipulation. The idea here is: when a value is selected/deselected I don't have access to the value itself, only the array of the current selected ones, so what I'm doing is:
- check if the value "all" was the one clicked, if it was deselected, remove everything, if was selected, add everything
- if another value was selected and because of it all the values are now selected, update the "all" to be selected too
- otherwise just add/remove the value
Let me know if is better now :)
packages/cluster-ui/src/columnsSelector/columnsSelector.module.scss, line 11 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
Default font-sizes and spacing can be reused from
packages/cluster-ui/src/core/index.module.scss
(.lavel
class uses it already)
Done.
packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 76 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
@maryliag , what is the main intend to store
columns
as a string? Would it be better to keep it in array of strings?
I thought it would be better to save as string on cache to save space. I did refactor the code to get the value from cache (which is a string) and then transform to array. This way the code is easier to understand.
packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 327 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
What "default" value means?
on the majority of cases "default" means select everything, but you can have some columns you want to ignore. For example, we don't want to show the column "database" by default, so if the value is "default" we will hide that column.
packages/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 97 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
nit. It might be more clear to rewrite it in more declarative way with filter + map functions and then get rid of duplicate values with Set.
return [ ...new Set(statementsState.data.statements .filter(s => !!s.key.key_data.database) .map(s => s.key.key_data.database) ).values() ]
Done.
packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 155 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
nit. wrap
if
statement in{ ... }
to comply with linting rules
Done.
packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 159 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
it might be refactored out from
filter
predicate:const displayCol = displayColumns.split(","); return columns.filter(option => { return displayCol.includes(option.name); }
Done.
112fe14
to
8ac9b2c
Compare
Information about database is now displayed on Statements page and Statements Details page. By default the column database is not displayed, but with the new column selector option, it can be selected. New filter allows to select statements based on database name. Closes: #33316 Release note (ui change): Display database name information on Statements page (hidden by default) and Statements Details page. New filter option for statements based on databases name. New option to select which columns to display on statements table. Release note (bug fix): Transaction page showing correct value for implicit txn.
8ac9b2c
to
54511c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 8 files at r2, 11 of 11 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @j-low, @koorosh, and @laurenbarker)
64251: changefeedccl: support arrays in avro encoding r=HonoreDB a=HonoreDB Arrays are one of a few remaining types we don't support in avro changefeeds. This serializes them. Sadly can't use @miretskiy's new optimization since we need to load multiple maps with the same key into an array. Release note (bug fix): Changefeeds on tables with array columns support avro 64559: kvserver: stop reconciling range count imbalance in the StoreRebalancer r=aayushshah15 a=aayushshah15 Previously, the `StoreRebalancer` would rebalance ranges off of a node if it had more than the average number of replicas compared to the cluster average. This was undesirable because the `StoreRebalancer` was not respecting the {over,under}fullness thresholds that the replicateQueue does, which could lead to redundant range relocations even in a well balanced cluster. This commit is an opinionated fix to prevent this behavior and an attempt to more clearly delineate the responsibilities of the replica rebalancing aspects of the `StoreRebalancer` and the `replicateQueue`. Noticed while looking into #64064. Release note: None 64614: ui: update cluster-ui version r=maryliag a=maryliag Update cluster-ui version to include changes made on cockroachdb/ui#294 and minor fix to handle case when no column is selected. Release note (ui change): cluster-ui updated. Showing information about database on Statement Page and ability to choose which columns to display. 64664: authors: add charlie to authors r=charlievieth a=charlievieth Release note: none 64665: authors: add Wade Waldron to Authors r=jlinder a=WadeWaldron Release note: None 64666: authors: add bryan@cockroachlabs.com to authors r=jlinder a=strangr525 release note: none 64668: authors: add theodore hyman to authors r=jlinder a=theodore-hyman release note: None Co-authored-by: Aaron Zinger <zinger@cockroachlabs.com> Co-authored-by: Aayush Shah <aayush.shah15@gmail.com> Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com> Co-authored-by: Charlie Vieth <charlie@cockroachlabs.com> Co-authored-by: Wade Waldron <wade@cockroachlabs.com> Co-authored-by: Bryan Kwon <bryan@cockroachlabs.com> Co-authored-by: Theodore HymHyman <theodore@cockroachlabs.com>
Information about database is now displayed on Statements page and Statements Details page.
By default the column database is not displayed, but with the new column selector option, it can be selected.
New filter allows to select statements based on database name.
New filter option for databases:
New column selector:
New column
Info on Statement Details
Closes: cockroachdb/cockroach#33316
Closes: cockroachdb/cockroach#59210
Release note (ui change): Display database name information on Statements
page (hidden by default) and Statements Details page.
New filter option for statements based on databases name.
New option to select which columns to display on statements table.
Release note (bug fix): Transaction page showing correct value for implicit txn
This change is